Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

style(scripts): Align gen-upgrade-proposal.sh with community forum release posts #9271

Merged
merged 3 commits into from
May 1, 2024

Conversation

gibson042
Copy link
Member

Fixes #9270

Description

Adopt variable names and interpolation patterns as seen in community forum posts such as cf. https://community.agoric.com/t/proposal-71-agoric-upgrade-14-interchain-stack-smart-wallet-upgrades/633#creating-an-emerynet-upgrade-proposal-for-rc1-3 . Also provides slightly more guidance regarding expected transaction options and where to look for past proposals.

Security Considerations

n/a

Scaling Considerations

n/a

Documentation Considerations

We might want to direct operators to this script, rather than duplicating its contents.

Testing Considerations

This script is well-exercised.

Upgrade Considerations

Script functionality must not change.

Copy link

cloudflare-workers-and-pages bot commented Apr 20, 2024

Deploying agoric-sdk with  Cloudflare Pages  Cloudflare Pages

Latest commit: 0bea601
Status: ✅  Deploy successful!
Preview URL: https://91a7eb63.agoric-sdk.pages.dev
Branch Preview URL: https://gibson-9270-gen-upgrade-prop.agoric-sdk.pages.dev

View logs


info="{\"binaries\":{\"any\":\"$ZIPURL//agoric-sdk-$COMMIT_ID?checksum=$checksum\"},\"source\":\"$ZIPURL?checksum=$checksum\"}"
UPGRADE_INFO="{\"binaries\":{\"any\":\"$ZIP_URL//agoric-sdk-${COMMIT_ID}?checksum=$CHECKSUM\"},\"source\":\"$ZIP_URL?checksum=$CHECKSUM\"}"
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This aligns with recent forum posts, but we could also quote all interpolated variables (both here and in future posts):

Suggested change
UPGRADE_INFO="{\"binaries\":{\"any\":\"$ZIP_URL//agoric-sdk-${COMMIT_ID}?checksum=$CHECKSUM\"},\"source\":\"$ZIP_URL?checksum=$CHECKSUM\"}"
UPGRADE_INFO="{\"binaries\":{\"any\":\"${ZIP_URL}//agoric-sdk-${COMMIT_ID}?checksum=${CHECKSUM}\"},\"source\":\"${ZIP_URL}?checksum=${CHECKSUM}\"}"

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As you wish. It may help readability, though is semantically identical, AFAIK.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It is. Such a change would only be for consistency and readability.

Comment on lines 15 to 21
echo "Verifying archive is at $ZIPURL..." 1>&2
echo "Verifying archive is at $ZIP_URL..." 1>&2
zipfile=$(mktemp)
trap 'rm -f "$zipfile"' EXIT
curl -L "$ZIPURL" -o "$zipfile"
curl -L "$ZIP_URL" -o "$zipfile"

echo "Generating SHA-256 checksum..." 1>&2
checksum=sha256:$(shasum -a 256 "$zipfile" | cut -d' ' -f1)
CHECKSUM=sha256:$(shasum -a 256 "$zipfile" | cut -d' ' -f1)
Copy link
Member Author

@gibson042 gibson042 Apr 20, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The code in forum posts is not assumed to be running inside a script, and so doesn't have this download-with-deferred-cleanup behavior. But unfortunately, that means it also doesn't have verification that the archive exists. We could get that by adding a HEAD request both here and in future posts, e.g.

echo "Verifying archive is at $ZIP_URL..." 1>&2
curl -fLI --no-progress-meter "$ZIP_URL" -o- > /dev/null

echo "Generating SHA-256 checksum..." 1>&2
CHECKSUM=sha256:$(curl -fL "$ZIP_URL" -o- | shasum -a 256 | cut -d' ' -f1)

Or just keeping the data (currently ~10 MB) in a variable:

ZIP_DATA=$(curl -fL "$ZIP_URL" -o-)
CHECKSUM=sha256:$(printf '%s' "$ZIP_DATA" | shasum -a 256 | cut -d' ' -f1)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd prefer using the -f flag to have curl fail fast:

echo "Verifying ${ZIP_URL} presence and generating SHA-256 checksum..." 1>&2
CHECKSUM=sha256:$(curl -fL "$ZIP_URL" -o- | shasum -a 256 | cut -d' ' -f1)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As noted above, the code in forum posts is not assumed to be running inside a script—so pipefail may not be active or even available (our instructions don't require bash). For example, here's what I see when running in a clean shell:

$ CHECKSUM=sha256:$(curl -L 'https://example.invalid/' -o- | shasum -a 256 | cut -d' ' -f1)
  % Total    % Received % Xferd  Average Speed   Time    Time     Time  Current
                                 Dload  Upload   Total   Spent    Left  Speed
  0     0    0     0    0     0      0      0 --:--:-- --:--:-- --:--:--     0
curl: (6) Could not resolve host: example.invalid
$ echo "CHECKSUM: $CHECKSUM"
CHECKSUM: sha256:e3b0c44298fc1c149afbf4c8996fb92427ae41e4649b934ca495991b7852b855

But I do like the idea of using -f, and have updated accordingly.

Copy link
Member

@turadg turadg left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks fine to me. I pushed a white space fix to pass lint.

Comment on lines 15 to 21
echo "Verifying archive is at $ZIPURL..." 1>&2
echo "Verifying archive is at $ZIP_URL..." 1>&2
zipfile=$(mktemp)
trap 'rm -f "$zipfile"' EXIT
curl -L "$ZIPURL" -o "$zipfile"
curl -L "$ZIP_URL" -o "$zipfile"

echo "Generating SHA-256 checksum..." 1>&2
checksum=sha256:$(shasum -a 256 "$zipfile" | cut -d' ' -f1)
CHECKSUM=sha256:$(shasum -a 256 "$zipfile" | cut -d' ' -f1)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd prefer using the -f flag to have curl fail fast:

echo "Verifying ${ZIP_URL} presence and generating SHA-256 checksum..." 1>&2
CHECKSUM=sha256:$(curl -fL "$ZIP_URL" -o- | shasum -a 256 | cut -d' ' -f1)


info="{\"binaries\":{\"any\":\"$ZIPURL//agoric-sdk-$COMMIT_ID?checksum=$checksum\"},\"source\":\"$ZIPURL?checksum=$checksum\"}"
UPGRADE_INFO="{\"binaries\":{\"any\":\"$ZIP_URL//agoric-sdk-${COMMIT_ID}?checksum=$CHECKSUM\"},\"source\":\"$ZIP_URL?checksum=$CHECKSUM\"}"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As you wish. It may help readability, though is semantically identical, AFAIK.

@gibson042 gibson042 requested a review from michaelfig April 26, 2024 23:26
@gibson042 gibson042 force-pushed the gibson-9270-gen-upgrade-proposal branch from cea17a2 to 0bea601 Compare May 1, 2024 17:50
@gibson042 gibson042 added the automerge:rebase Automatically rebase updates, then merge label May 1, 2024
@mergify mergify bot merged commit a2c6d29 into master May 1, 2024
75 checks passed
@mergify mergify bot deleted the gibson-9270-gen-upgrade-proposal branch May 1, 2024 18:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
automerge:rebase Automatically rebase updates, then merge
Projects
None yet
Development

Successfully merging this pull request may close these issues.

gen-upgrade-proposal.sh variable name spelling is inconsistent with forum posts
3 participants